Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix no-turbolinks ci #244

Merged
merged 18 commits into from
Feb 1, 2016
Merged

Fix no-turbolinks ci #244

merged 18 commits into from
Feb 1, 2016

Conversation

justin808
Copy link
Member

Root cause of failures on CI might be that if gem turbolinks is installed, then we don't run the handlers for no-turbolinks.

However, that should only be affected by requiring of turbolinks.

Maybe we should change:

  1. Don't worry if the Gem turbolinks is installed.
  2. Only check the JS side if turbolinks is running.

Review on Reviewable

@justin808
Copy link
Member Author

@robwise You might want to refactor this a tiny bit.

@robwise
Copy link
Contributor

robwise commented Jan 30, 2016

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

rakelib/dummy_apps.rake, line 11 [r1] (raw file):
there's a helper in task_helpers.rb that we should use for this to keep it DRY, not sure why I didn't use that in the first place

rakelib/run_rspec.rake, line 26 [r1] (raw file):
why are we bundle installing after we run the tests?

Comments from the review on Reviewable.io

@justin808
Copy link
Member Author

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

rakelib/dummy_apps.rake, line 11 [r1] (raw file):
agree!

rakelib/run_rspec.rake, line 26 [r1] (raw file):
return to prior state or else gemfile.lock is dirty

Comments from the review on Reviewable.io

@robwise
Copy link
Contributor

robwise commented Jan 30, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

rakelib/run_rspec.rake, line 26 [r1] (raw file):
ah, makes sense! I think this calls for a comment here because otherwise we might forget or someone else might not know etc.

Comments from the review on Reviewable.io

justin808 and others added 7 commits January 29, 2016 23:45
@justin808
Allow DISABLE_TURBOLINKS to be defined but blank
b1e9eaf 
And don't bundle with the option DISABLE_TURBOLINKS on.
@justin808
Be sure to run npm install in spec/dummy
bdbe79f
@robwise
Disable symlinking of node package
8b29022
@justin808
Don't specify js driver
519fba5
@justin808
Trying update dependencies, fails server rendering
8e37503 
ROR in SERVER PRERENDERING
Encountered error: "TypeError: Cannot read property 'name' of undefined"
when prerendering N/A (server_render_js called) with props:
js_code was:
(function() {
  var htmlResult = '';
  var consoleReplayScript = '';
  var hasErrors = false;

  try {
    htmlResult =
      (function() {
        return
ReactOnRails.getComponent('HelloString').component.world();
      })();
  } catch(e) {
    htmlResult = ReactOnRails.handleError({e: e, name: null,
      jsCode:
'ReactOnRails.getComponent(\'HelloString\').component.world()',
serverSide: true});
    hasErrors = true;
  }

  consoleReplayScript = ReactOnRails.buildConsoleReplay();

  return JSON.stringify({
      html: htmlResult,
      consoleReplayScript: consoleReplayScript,
      hasErrors: hasErrors
  });

})()

Object.<anonymous> ((execjs):27066:58)
Object.<anonymous> ((execjs):27075:31)
__webpack_require__ ((execjs):31:30)
Object.<anonymous> ((execjs):26981:15)
__webpack_require__ ((execjs):31:30)
Object.<anonymous> ((execjs):5355:24)
__webpack_require__ ((execjs):31:30)
Object.<anonymous> ((execjs):59:19)
__webpack_require__ ((execjs):31:30)
(execjs):51:18
@justin808
Add shinkrwraps, fails locally
d1b44d6 
Probably not related to the react version

Encountered error: "TypeError: Cannot read property 'name' of undefined"
when prerendering HelloWorldWithLogAndThrow with props:
{"helloWorldData":{"name":"Mr. Server Side Rendering"}}
js_code was:
(function() {
  var props = {"helloWorldData":{"name":"Mr. Server Side Rendering"}};
  return ReactOnRails.serverRenderReactComponent({
    name: 'HelloWorldWithLogAndThrow',
    domNodeId: 'HelloWorldWithLogAndThrow-react-component-0',
    props: props,
    trace: true,
    location: '/server_side_log_throw_raise'
  });
})()

Object.<anonymous> ((execjs):27066:58)
Object.<anonymous> ((execjs):27075:31)
__webpack_require__ ((execjs):31:30)
Object.<anonymous> ((execjs):26981:15)
__webpack_require__ ((execjs):31:30)
Object.<anonymous> ((execjs):5355:24)
__webpack_require__ ((execjs):31:30)
Object.<anonymous> ((execjs):59:19)
__webpack_require__ ((execjs):31:30)
(execjs):51:18
@justin808
Allow debugging errors in server gen js
ea16151 
This change prints a useful message and outputs the file with the issue.
@justin808 justin808 mentioned this pull request Jan 31, 2016
Closed
@justin808
Copy link
Member Author

Debugging details:

#246 (comment)

justin808 referenced this pull request in reduxjs/redux Jan 31, 2016
@gaearon
Disable envification warning if Function.name is not supported
e2e14d2 
Fixes #1311
@justin808
Copy link
Member Author

reduxjs/redux@e2e14d2#diff-1fdf421c05c1140f6d71444ea2b27638

@justin808 justin808 mentioned this pull request Jan 31, 2016
Closed
@justin808
Copy link
Member Author

See reduxjs/redux#1335

@robwise
Copy link
Contributor

robwise commented Jan 31, 2016

Reviewed 8 of 8 files at r2, 1 of 4 files at r4, 4 of 4 files at r5.
Review status: 17 of 18 files reviewed at latest revision, 2 unresolved discussions.

rakelib/dummy_apps.rake, line 10 [r1] (raw file):
can we put this back now or this was breaking CI also?

Comments from the review on Reviewable.io

@robwise
Copy link
Contributor

robwise commented Jan 31, 2016

Reviewed 1 of 4 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

@robwise
Copy link
Contributor

robwise commented Feb 1, 2016

Reviewed 4 of 4 files at r7.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

@justin808
Copy link
Member Author

👍😎

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

@justin808
Copy link
Member Author

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

rakelib/dummy_apps.rake, line 10 [r1] (raw file):
we just don't need separate bundle install

Comments from the review on Reviewable.io

@alex35mil
Copy link
Member

:lgtm_strong:

Review status: 20 of 21 files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

justin808 added a commit that referenced this pull request Feb 1, 2016
@justin808
Merge pull request #244 from shakacode/fix-ci-no-turbolinks
33a9262 
Fix no-turbolinks ci
@justin808 justin808 merged commit 33a9262 into master Feb 1, 2016
@justin808 justin808 deleted the fix-ci-no-turbolinks branch February 1, 2016 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@justin808 @robwise @alex35mil